Updated VPA admission-controller to have adjustable minimum TLS version and TLS ciphers #6625

allenmun197 · 2024-03-12T18:22:52Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR allows the VPA admission-controller to have an adjustable minimum TLS version and ciphersuites. This will help people using VPA to be more secure as you can now set a minimum TLS version of 1.3 or only allow strong ciphers.

Which issue(s) this PR fixes:

Fixes #6624

Special notes for your reviewer:

I don't know the best way to create a mapping of ciphers/tls_versions to convert the string from the flag to the accepted uint16 accepted by the server params. If there is a better/cleaner solution I would be very much open to changing the code.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla · 2024-03-12T18:22:56Z

The committers listed above are authorized under a signed CLA.

✅ login: allenmun197 / name: allenmun (2cd5884)

mwielgus

/lgtm
/approve

allenmun197 · 2024-03-15T19:28:15Z

@mwielgus fixed the formatting issue, should be good now

voelzmo

Thanks for immediately providing a PR for your issue! I have a few requests regarding the ux, where I think we can be a bit nicer to users.

voelzmo · 2024-03-16T19:05:43Z

vertical-pod-autoscaler/pkg/admission-controller/README.md

@@ -32,6 +32,8 @@ up the changes: ```sudo systemctl restart kubelet.service```
   for pods on their creation & updates.
 1. You can specify a path for it to register as a part of the installation process
   by setting `--register-by-url=true` and passing `--webhook-address` and `--webhook-port`.
+1. You can specify a minimum TLS version with `--min-tls-version` with acceptable values being `tls1_0`, `tls1_1`, `tls1_2` (default), or `tls_13`.


Suggested change

1. You can specify a minimum TLS version with `--min-tls-version` with acceptable values being `tls1_0`, `tls1_1`, `tls1_2` (default), or `tls_13`.

1. You can specify a minimum TLS version with `--min-tls-version` with acceptable values being `tls1_0`, `tls1_1`, `tls1_2` (default), or `tls1_3`.

updated typo

voelzmo · 2024-03-16T19:11:04Z

vertical-pod-autoscaler/pkg/admission-controller/config.go

+	case "tls1_3":
+		tlsVersion = tls.VersionTLS13
+	default:
+		tlsVersion = tls.VersionTLS12


This is somewhat dangerous, IMHO. This code allows for arbitrary strings to be set for --min-tls-version and any typo will not result in an error, but in TLS1.2 being set. Users will never realize they're not getting what they intended. For example, if I'm setting --min-tls-version=1.3, admission-controller will silently configure TLS1.2. I think we should make it explicit to the user that they entered a value that is unknown and fail to start.

updated code to default to tls1_2 and only accept tls1_2 and tls1_3. If a different value is given, we now throw a fatal exception. Error now looks like

F0318 15:31:04.356258 1 config.go:66] Unable to determine value for --min-tls-version (tls1_1), must be either tls1_2 or tls1_3

voelzmo · 2024-03-16T19:17:42Z

vertical-pod-autoscaler/pkg/admission-controller/config.go

+	}
+
+	switch minTlsVersion {
+	case "tls1_0":


Do we have a good reason to "go back" and allow for less secure TLS configurations than we do now? If not, let's not include TLS1.0 and TLS1.1 here.

removed TLS 1.0 and TLS 1.1

voelzmo · 2024-03-16T19:27:00Z

vertical-pod-autoscaler/pkg/admission-controller/main.go

@@ -59,6 +59,8 @@ var (
 		tlsCertFile:   flag.String("tls-cert-file", "/etc/tls-certs/serverCert.pem", "Path to server certificate PEM file."),
 		tlsPrivateKey: flag.String("tls-private-key", "/etc/tls-certs/serverKey.pem", "Path to server certificate key PEM file."),
 	}
+	ciphers       = flag.String("tls-ciphers", "", "A comma-separated or colon-separated list of ciphers to accept.  Only works when min-tls-version is set to tls1_2 or below.")
+	minTlsVersion = flag.String("min-tls-version", "tls1_2", "The minimum TLS version to accept.  Defaults to tls1_2.")


We should list all allowed values here, to make it clear which values are valid without taking a look at the implementation.

updated description

allenmun197 · 2024-03-18T15:38:46Z

@voelzmo Updated the PR, made the changes. LMK if there is anything else that needs to be updated.

… --tls-ciphers

voelzmo · 2024-03-19T09:34:57Z

Looks great, thanks!
/lgtm

k8s-ci-robot · 2024-03-19T09:35:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: allenmun197, mwielgus, voelzmo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [mwielgus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

robbiezhang · 2024-06-20T00:40:32Z

@voelzmo , is it fine to cherry-pick this change to vpa 1.1 release?

voelzmo · 2024-06-21T08:42:39Z

@robbiezhang I don't think we cherry-pick new features to releases. We probably would rather release a 1.2.0
@kwiesmueller @raywainman WDYT?

robbiezhang · 2024-06-21T13:54:38Z

@voelzmo thanks for the reply. That makes sense. Wondering what's the release cadence and 1.2 release timeline?

kwiesmueller · 2024-06-21T14:17:49Z

Agreed. A new version would be necessary for features.

raywainman · 2024-06-21T17:00:07Z

We generally try and cut releases roughly in line with every minor version release.

That being said there really isn't anything preventing us from a doing another release (except bandwidth from one of the owners!)

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 12, 2024

k8s-ci-robot requested a review from jbartosik March 12, 2024 18:22

k8s-ci-robot added the area/vertical-pod-autoscaler label Mar 12, 2024

k8s-ci-robot requested a review from kgolab March 12, 2024 18:22

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 12, 2024

allenmun197 force-pushed the ciphers branch from 026923d to 003830d Compare March 12, 2024 18:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 12, 2024

mwielgus approved these changes Mar 14, 2024

View reviewed changes

k8s-ci-robot assigned mwielgus Mar 14, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 14, 2024

allenmun197 force-pushed the ciphers branch from 003830d to 265a987 Compare March 15, 2024 19:06

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2024

voelzmo suggested changes Mar 16, 2024

View reviewed changes

allenmun197 force-pushed the ciphers branch from 265a987 to af7d2d1 Compare March 18, 2024 15:35

updated admission-controller to have adjustable --min-tls-version and…

2cd5884

… --tls-ciphers

allenmun197 force-pushed the ciphers branch from af7d2d1 to 2cd5884 Compare March 18, 2024 15:42

k8s-ci-robot assigned voelzmo Mar 19, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2024

voelzmo approved these changes Mar 19, 2024

View reviewed changes

k8s-ci-robot merged commit 24fbe6e into kubernetes:master Mar 19, 2024
6 checks passed

voelzmo mentioned this pull request Jun 26, 2024

"vpa-admission-controller" getting into CrashLoopBackOff in result to PR: #6665 #6977

Closed

raywainman mentioned this pull request Aug 6, 2024

Release VPA 1.2.0 #7098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated VPA admission-controller to have adjustable minimum TLS version and TLS ciphers #6625

Updated VPA admission-controller to have adjustable minimum TLS version and TLS ciphers #6625

allenmun197 commented Mar 12, 2024

linux-foundation-easycla bot commented Mar 12, 2024 •

edited

Loading

mwielgus left a comment

allenmun197 commented Mar 15, 2024

voelzmo left a comment

voelzmo Mar 16, 2024

allenmun197 Mar 18, 2024

voelzmo Mar 16, 2024

allenmun197 Mar 18, 2024

voelzmo Mar 16, 2024

allenmun197 Mar 18, 2024

voelzmo Mar 16, 2024

allenmun197 Mar 18, 2024

allenmun197 commented Mar 18, 2024

voelzmo commented Mar 19, 2024

k8s-ci-robot commented Mar 19, 2024

robbiezhang commented Jun 20, 2024

voelzmo commented Jun 21, 2024

robbiezhang commented Jun 21, 2024

kwiesmueller commented Jun 21, 2024

raywainman commented Jun 21, 2024

	1. You can specify a minimum TLS version with `--min-tls-version` with acceptable values being `tls1_0`, `tls1_1`, `tls1_2` (default), or `tls_13`.
	1. You can specify a minimum TLS version with `--min-tls-version` with acceptable values being `tls1_0`, `tls1_1`, `tls1_2` (default), or `tls1_3`.

Updated VPA admission-controller to have adjustable minimum TLS version and TLS ciphers #6625

Updated VPA admission-controller to have adjustable minimum TLS version and TLS ciphers #6625

Conversation

allenmun197 commented Mar 12, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla bot commented Mar 12, 2024 • edited Loading

mwielgus left a comment

Choose a reason for hiding this comment

allenmun197 commented Mar 15, 2024

voelzmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenmun197 commented Mar 18, 2024

voelzmo commented Mar 19, 2024

k8s-ci-robot commented Mar 19, 2024

robbiezhang commented Jun 20, 2024

voelzmo commented Jun 21, 2024

robbiezhang commented Jun 21, 2024

kwiesmueller commented Jun 21, 2024

raywainman commented Jun 21, 2024

linux-foundation-easycla bot commented Mar 12, 2024 •

edited

Loading